Skip to content

Add clickable children to LinkTooltip#209

Open
saturninoabril wants to merge 2 commits intomasterfrom
add-clickable-children-linktooltip
Open

Add clickable children to LinkTooltip#209
saturninoabril wants to merge 2 commits intomasterfrom
add-clickable-children-linktooltip

Conversation

@saturninoabril
Copy link
Copy Markdown
Contributor

Summary

  • Added clickable children similar to how it's used by GitHub plugin
  • Bumped version to 0.11.1

Ticket Link

None, ref: mattermost/mattermost#36056 (comment)

Screenshot

Screenshot 2026-04-14 at 9 17 16 AM

@saturninoabril saturninoabril added the 2: Dev Review Requires review by a core committer label Apr 14, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 14, 2026

📝 Walkthrough

Walkthrough

The Mattermost plugin manifest version was bumped from 0.10.3 to 0.11.1. The link tooltip component was refactored to only render for example.com hosts, and its markup, styling keys, and localized text were expanded to include a header, linked title, shared-via text, and description with safe external link attributes.

Changes

Cohort / File(s) Summary
Version Manifest
plugin.json
Version field updated from 0.10.30.11.1.
Tooltip Component
webapp/src/components/link_tooltip/link_tooltip.jsx
Rendering now parses href and only renders for example.com (or subdomains). Markup restructured into header (icon + message) and body (linked title, shared-via, description). New data-testid, target="_blank", rel="noopener noreferrer" on anchors. Style keys replaced/added (container, header, body, titleLink, sharedVia, description).
i18n — English
webapp/i18n/en.json
Added tooltip.title, tooltip.sharedVia, and tooltip.description translation keys.
i18n — Spanish
webapp/i18n/es.json
Added tooltip.title, tooltip.sharedVia, and tooltip.description translation keys.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding clickable children (links) to the LinkTooltip component, which aligns with the primary modifications in the webapp component.
Description check ✅ Passed The description is directly related to the changeset, explaining that clickable children were added to LinkTooltip and the version was bumped, which matches the file modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch add-clickable-children-linktooltip

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@webapp/src/components/link_tooltip/link_tooltip.jsx`:
- Around line 37-65: Add the new i18n keys introduced in link_tooltip.jsx —
tooltip.title, tooltip.sharedVia, and tooltip.description — to the English
locale catalogue (en JSON) with the same text as their defaultMessage values
(e.g., "Demo Link Preview", "Shared via {link}", "This is a sample description.
Visit {link} for more details.") so translators can see them; ensure the keys
match exactly and run the i18n extractor/update process if your workflow
requires it.
- Around line 12-13: Replace the naive substring check on this.props.href in
link_tooltip.jsx with a proper hostname comparison: parse the URL using the URL
constructor inside a try/catch (to handle invalid URLs), extract url.hostname,
and return null unless hostname === 'example.com' (or whatever exact domain you
intend to allow); remove the this.props.href.includes(...) check and use this
new parsed-hostname validation so hosts like "example.com.evil.test" or URIs
with query params won't pass.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9a2d8c39-07f3-456c-818c-d49e23a2532d

📥 Commits

Reviewing files that changed from the base of the PR and between 2386640 and aba3eed.

📒 Files selected for processing (2)
  • plugin.json
  • webapp/src/components/link_tooltip/link_tooltip.jsx

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
webapp/src/components/link_tooltip/link_tooltip.jsx (1)

61-63: Consider using the actual hostname instead of hardcoding.

Line 62 hardcodes 'example.com' for the "Shared via" link text. If a subdomain like sub.example.com is used, the display would be slightly misleading. Since you already have the parsed hostname, you could use it here for accuracy.

♻️ Suggested change
                                    <a
                                        href={this.props.href}
                                        target='_blank'
                                        rel='noopener noreferrer'
                                        data-testid='demo-tooltip-shared-via-link'
                                    >
-                                        {'example.com'}
+                                        {hostname}
                                    </a>

Note: This would require moving hostname to be accessible in the render scope (it already is from the parsing block at the top of render()).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@webapp/src/components/link_tooltip/link_tooltip.jsx` around lines 61 - 63,
The link text currently hardcodes 'example.com' in the render output; change it
to use the parsed hostname variable (hostname) so the "Shared via" anchor
displays the real host (including subdomains). Make hostname available in render
scope (it already exists in the parsing block) and replace the literal string in
the JSX anchor text with the hostname variable used when building the link.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@webapp/src/components/link_tooltip/link_tooltip.jsx`:
- Around line 61-63: The link text currently hardcodes 'example.com' in the
render output; change it to use the parsed hostname variable (hostname) so the
"Shared via" anchor displays the real host (including subdomains). Make hostname
available in render scope (it already exists in the parsing block) and replace
the literal string in the JSX anchor text with the hostname variable used when
building the link.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4b08f60c-e44d-47d3-a353-6ee9dba8bf68

📥 Commits

Reviewing files that changed from the base of the PR and between aba3eed and b6a435a.

📒 Files selected for processing (3)
  • webapp/i18n/en.json
  • webapp/i18n/es.json
  • webapp/src/components/link_tooltip/link_tooltip.jsx
✅ Files skipped from review due to trivial changes (2)
  • webapp/i18n/es.json
  • webapp/i18n/en.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3: QA Review Requires review by a QA tester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant